[RPC] require valid URL scheme on budget commands #950

CaveSpectre11 · 2019-07-13T22:00:32Z

Release Notes

[RPC] Require valid URL schemes (URLs starting with "http" or "https") on preparebudget and submitbudget.

Details

The budget proposal system did not have any checks on the URL aside of string length. This allowed for the scheme to be left off the URL; which on some browsers, resulted in the link to fail when clicked:

gio: file:///home/CaveSpectre/forum.pivx.org/index.php%3Fthreads/bizdev-proposal-john-m.285: 
Error when getting information for file “/home/CaveSpectre/forum.pivx.org/index.php?
threads/bizdev-proposal-john-m.285”: No such file or directory

The included PR changes the submitbudget and preparebudget rpc commands to prevent inadvertent omissions of the URL scheme (http:// or https://).

Qt changes to fixup the existing & historical budget URLs will be submitted as a separate PR after the merging of #954 .

(This PR originally included the Qt changes, and as a result some of the below discussion relates to that, before the decision was made to back out the Qt changes until a later date)

Warrows

Concept ACK, however the use of std regex seems to be making a travis job fail.

src/masternode-budget.h

CaveSpectre11 · 2019-07-14T13:38:10Z

I originally thought that regex failure in the zerocoin test was something unrelated; or related to the zerocoin changes. But now that I look at it....

@Warrows , do you happen to know why that test uses gcc 4.8? regex should be stable in 4.9, and most of the builds appear to be using 7.4. Are we restricted to supporting 4.8?

Warrows · 2019-07-14T14:19:24Z

That's a question better suited for @Fuzzbawls

CaveSpectre11 · 2019-07-14T16:16:58Z

I should be able to conditionalize it so that regex_match() just returns true on builds prior to gcc 4.9, which would effectively fix the travis test, but have no impact on the releases. I will wait for @Fuzzbawls to suggest direction.

#include <regex>

#if __cplusplus >= 201103L &&                             \
    (!defined(__GLIBCXX__) || (__cplusplus >= 201402L) || \
        (defined(_GLIBCXX_REGEX_DFS_QUANTIFIERS_LIMIT) || \
         defined(_GLIBCXX_REGEX_STATE_LIMIT)           || \
             (defined(_GLIBCXX_RELEASE)                && \
             _GLIBCXX_RELEASE > 4)))
#  define HAVE_WORKING_REGEX 1
#else
#  define HAVE_WORKING_REGEX 0
#endif

It'll take a little toying to get it right

Fuzzbawls · 2019-07-15T00:48:27Z

Concept ACK, but would need to do this without relying on std::regex so as to avoid bumping the minimum supported ABI.

maybe have a look at using std::string::find instead.

CaveSpectre11 · 2019-07-15T00:54:31Z

std::string::find

It wouldn't be as robust; but yes, searching for "://" would at least solve the inadvertent and common omission.

CaveSpectre11 · 2019-07-15T02:12:25Z

Quick test log of the rpc restrictions. logically, "h://" and "H://" pass, but we at least force [hH]+:// which should make it better.

src/masternode-budget.h

Fuzzbawls

ACK d92b438

random-zebra · 2019-07-26T11:39:15Z

I don't particularly like this solution.
Think it would be better to use tolower() (even if locale dependent and requiring some additional changes in the lint script) on everything before :// and compare with the allowed prefixes.

Anyway, would code the logic in a separate utility function and call that from submitbudget and preparebudget to avoid more code duplication (those two functions already share way too much code in my opinion).

CaveSpectre11 · 2019-07-26T12:14:19Z

Anyway, would code the logic in a separate utility function and call that from submitbudget and preparebudget to avoid more code duplication (those two functions already share way too much code in my opinion).

The original design had a utility function for adding the scheme; but the code check in the rpc code was less complicated. I think I agree that a utility function is the way to go; especially given the premise of the last round of changes (adding more URL scheme) validation. Pulling it together into a utility function centralizes where future code can go, if the need to have further validation of the URL is desired later.

Think it would be better to use tolower() (even if locale dependent and requiring some additional changes in the lint script) on everything before :// and compare with the allowed prefixes.

I will go ahead and pull the logic into a function; and you and @Fuzzbawls can hash out the pros and cons of tolower() and finish it out when that direction is determined.

CaveSpectre11 · 2019-07-26T19:20:26Z

@Fuzzbawls @random-zebra; I have submitted a much broader PR (#964 ) that encompasses this PR, with modifications to the logic to hopefully account for both your concerns.

That PR does supersede this one, and if that solution is preferred, this one should be closed.

src/rpc/budget.cpp

CaveSpectre11 · 2019-08-02T02:12:36Z

Ok; I just totally jacked this one up trying to merge in #965. I'll try to figure it out tomorrow.

983d97a [Refactor] Combine parameter checking of budget commands (Cave Spectre) Pull request description: ### **Release notes** - [Refactor] Move common budget parameter checking code into a function ### **Details** The code in both `preparebudget` and `submitbudget` used the same parameter checking. This pulls that parameter checking and loading into a common routine used by both; for ease of improvements and adjustments to the parameter checking... so the risk of not adjusting them consistently in both routines is removed. ### **Note** The early discussion below is based on a combined PR that addressed this, plus #950 and a PR to be named later. The decision was made to split and focus; so the discussions can be independent of each other and refactor as needed. ACKs for top commit: Fuzzbawls: ACK 983d97a random-zebra: ACK 983d97a Tree-SHA512: a88eb41e06d1936db77f1cd9c4f04360b446c48d27652d57dfd6e4b119140d56e24b24e00e0ac48890cc78d4130f8f688a711f74899f9c9301d1aba22dff5315

Mrs-X · 2019-08-05T19:18:40Z

Just some random thoughts:

checkBudgetInputs() was already there, so why did you copy that code into the methods?
the canonical form of a scheme is lowercase, so I second forcing http:// or https://

CaveSpectre11 · 2019-08-05T20:26:06Z

checkBudgetInputs() was already there, so why did you copy that code into the methods?

#964 was meant as a replacement to this one originally when I submitted it. But it was decided they should be separate. This one to deal with the scheme check, 964 for the refactor only; and 965 for the fixes on the existing code. The refactoring was what combined the common code in the prepare and submit into checkBudgetInputs().

Then to make it more confusing; 964's change was cherry picked into 965; and both were picked into this one... presuming the merge order to mainline would be 964 first; then 965, then this one... and thus the rebasing legwork was already done.

CaveSpectre11 · 2019-08-05T23:12:03Z

rebased due to conflict from on budget.cpp

random-zebra · 2019-08-08T02:55:55Z

Needs rebase after #965 merge.

CaveSpectre11 · 2019-08-08T12:02:28Z

Needs rebase after #965 merge.

Merged through github; will squash that commit when I get home.

Fuzzbawls

functionally good, just a comment move

src/utilstrencodings.cpp

Fuzzbawls

ACK 009bd63

src/utilstrencodings.cpp

random-zebra

utACK 009bd63 and merging...

009bd63 [RPC] require valid URL scheme on budget commands (Cave Spectre) Pull request description: ### **Release Notes** [RPC] Require valid URL schemes (URLs starting with "http" or "https") on preparebudget and submitbudget. ### **Details** The budget proposal system did not have any checks on the URL aside of string length. This allowed for the scheme to be left off the URL; which on some browsers, resulted in the link to fail when clicked: ![image](https://user-images.githubusercontent.com/36988814/61176879-69c97a80-a596-11e9-9307-cbeeb210e133.png) ``` gio: file:///home/CaveSpectre/forum.pivx.org/index.php%3Fthreads/bizdev-proposal-john-m.285: Error when getting information for file “/home/CaveSpectre/forum.pivx.org/index.php? threads/bizdev-proposal-john-m.285”: No such file or directory ``` The included PR changes the `submitbudget` and `preparebudget` rpc commands to prevent inadvertent omissions of the URL scheme (http:// or https://). Qt changes to fixup the existing & historical budget URLs will be submitted as a separate PR after the merging of #954 . ![image](https://user-images.githubusercontent.com/36988814/61176955-017b9880-a598-11e9-8df0-f4afe1ccdf7d.png) _(This PR originally included the Qt changes, and as a result some of the below discussion relates to that, before the decision was made to back out the Qt changes until a later date)_ ACKs for top commit: Fuzzbawls: ACK 009bd63 random-zebra: utACK 009bd63 and merging... Tree-SHA512: c08538af442ce1177cae8684bc71e7a6148a3cdb462c1e442e21e2bd5aa91b0ee8417a1692073d9db2a356f86785ab85b2c2cbcf6c681c24c2cd4921094c8ff3

@NoobieDev12

13735da [QA] Add unit test for validateURL() (random-zebra) a62f88a [Util] Use C++11 std::regex for validateURL() function (random-zebra) Pull request description: We validate (user side) the URL of budget proposals before their submission via RPC, and [#soon](#2406) via the GUI. As noted by @NoobieDev12 in [this comment](#2406 (comment)), this URL validation is too limited. Essentially we are only checking that the string begins with the protocol. This PR changes the implementation of the function `validateURL()` , using C++11 `std::regex` (which was the original intention of #950, but discarded to avoid bumping the minimum supported ABI at the time). I've used the following pattern to match `^(https?)://[^\s/$.?#][^\s]*[^\s/.]\.[^\s/.][^\s]*[^\s.]$`, which is far from a complete validation, but it should cover the basic cases. We can refine it in the future if need be. Unit tests included. ACKs for top commit: furszy: nice 👌 , utACK 13735da Tree-SHA512: 436bd7eeacd80e528f8cabf4b87ad7435369801cd4050a136715bc982258ab7cecd2c32d0dd430d7e7b43e41729dc2660fbb8e8f4f9a796867ce5ad206df3e66

Warrows reviewed Jul 14, 2019

View reviewed changes

src/masternode-budget.h Outdated Show resolved Hide resolved

CaveSpectre11 force-pushed the budget-url branch from de41d81 to 312a333 Compare July 14, 2019 14:20

CaveSpectre11 force-pushed the budget-url branch 2 times, most recently from 32d43ae to 8768556 Compare July 15, 2019 02:08

Fuzzbawls requested a review from Mrs-X July 22, 2019 10:53

akshaynexus reviewed Jul 23, 2019

View reviewed changes

src/masternode-budget.h Outdated Show resolved Hide resolved

CaveSpectre11 changed the title ~~[Qt] [RPC] Fixup budget proposal URLs that are lacking scheme~~ [RPC] require http:// or https:// on budget commands Jul 26, 2019

CaveSpectre11 force-pushed the budget-url branch from def3dc5 to d92b438 Compare July 26, 2019 01:30

Fuzzbawls changed the title ~~[RPC] require http:// or https:// on budget commands~~ [RPC] require valid URL scheme on budget commands Jul 26, 2019

Fuzzbawls added the RPC label Jul 26, 2019

Fuzzbawls added this to the 4.0.0 milestone Jul 26, 2019

Fuzzbawls added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jul 26, 2019

Fuzzbawls assigned CaveSpectre11 Jul 26, 2019

Fuzzbawls previously approved these changes Jul 26, 2019

View reviewed changes

CaveSpectre11 dismissed Fuzzbawls’s stale review via 2c87d35 July 26, 2019 12:18

CaveSpectre11 force-pushed the budget-url branch from d92b438 to 2c87d35 Compare July 26, 2019 12:18

CaveSpectre11 force-pushed the budget-url branch from 66cb6c6 to ea79cad Compare July 26, 2019 14:32

CaveSpectre11 mentioned this pull request Jul 26, 2019

[Refactor] Combine parameter checking of budget commands #964

Merged

random-zebra reviewed Aug 1, 2019

View reviewed changes

src/rpc/budget.cpp Outdated Show resolved Hide resolved

CaveSpectre11 force-pushed the budget-url branch from e53f352 to c5089fb Compare August 1, 2019 02:31

random-zebra reviewed Aug 1, 2019

View reviewed changes

src/rpc/budget.cpp Outdated Show resolved Hide resolved

src/rpc/budget.cpp Outdated Show resolved Hide resolved

src/rpc/budget.cpp Outdated Show resolved Hide resolved

CaveSpectre11 mentioned this pull request Aug 2, 2019

[RPC] Correct issues with budget commands #965

Merged

CaveSpectre11 force-pushed the budget-url branch from 4517646 to 35d521c Compare August 2, 2019 10:41

CaveSpectre11 force-pushed the budget-url branch from 35d521c to 39d52a6 Compare August 5, 2019 23:10

CaveSpectre11 force-pushed the budget-url branch from a4bdfe6 to aec8ec4 Compare August 9, 2019 01:29

Fuzzbawls requested changes Aug 9, 2019

View reviewed changes

src/utilstrencodings.cpp Outdated Show resolved Hide resolved

[RPC] require valid URL scheme on budget commands

009bd63

CaveSpectre11 force-pushed the budget-url branch from aec8ec4 to 009bd63 Compare August 9, 2019 10:14

Fuzzbawls approved these changes Aug 9, 2019

View reviewed changes

random-zebra reviewed Aug 9, 2019

View reviewed changes

src/utilstrencodings.cpp Show resolved Hide resolved

random-zebra approved these changes Aug 9, 2019

View reviewed changes

random-zebra merged commit 009bd63 into PIVX-Project:master Aug 9, 2019

CaveSpectre11 deleted the budget-url branch August 9, 2019 21:52

random-zebra modified the milestones: 4.0.0, 3.4.0 Aug 25, 2019

Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Aug 26, 2019

random-zebra mentioned this pull request Sep 8, 2021

[Util] Better URL validation #2545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RPC] require valid URL scheme on budget commands #950

[RPC] require valid URL scheme on budget commands #950

CaveSpectre11 commented Jul 13, 2019 •

edited

Loading

Warrows left a comment

CaveSpectre11 commented Jul 14, 2019

Warrows commented Jul 14, 2019

CaveSpectre11 commented Jul 14, 2019

Fuzzbawls commented Jul 15, 2019

CaveSpectre11 commented Jul 15, 2019

CaveSpectre11 commented Jul 15, 2019

Fuzzbawls left a comment

random-zebra commented Jul 26, 2019 •

edited

Loading

CaveSpectre11 commented Jul 26, 2019

CaveSpectre11 commented Jul 26, 2019

CaveSpectre11 commented Aug 2, 2019

Mrs-X commented Aug 5, 2019

CaveSpectre11 commented Aug 5, 2019

CaveSpectre11 commented Aug 5, 2019

random-zebra commented Aug 8, 2019

CaveSpectre11 commented Aug 8, 2019 •

edited

Loading

Fuzzbawls left a comment

Fuzzbawls left a comment

random-zebra left a comment

[RPC] require valid URL scheme on budget commands #950

[RPC] require valid URL scheme on budget commands #950

Conversation

CaveSpectre11 commented Jul 13, 2019 • edited Loading

Release Notes

Details

Warrows left a comment

Choose a reason for hiding this comment

CaveSpectre11 commented Jul 14, 2019

Warrows commented Jul 14, 2019

CaveSpectre11 commented Jul 14, 2019

Fuzzbawls commented Jul 15, 2019

CaveSpectre11 commented Jul 15, 2019

CaveSpectre11 commented Jul 15, 2019

Fuzzbawls left a comment

Choose a reason for hiding this comment

random-zebra commented Jul 26, 2019 • edited Loading

CaveSpectre11 commented Jul 26, 2019

CaveSpectre11 commented Jul 26, 2019

CaveSpectre11 commented Aug 2, 2019

Mrs-X commented Aug 5, 2019

CaveSpectre11 commented Aug 5, 2019

CaveSpectre11 commented Aug 5, 2019

random-zebra commented Aug 8, 2019

CaveSpectre11 commented Aug 8, 2019 • edited Loading

Fuzzbawls left a comment

Choose a reason for hiding this comment

Fuzzbawls left a comment

Choose a reason for hiding this comment

random-zebra left a comment

Choose a reason for hiding this comment

CaveSpectre11 commented Jul 13, 2019 •

edited

Loading

random-zebra commented Jul 26, 2019 •

edited

Loading

CaveSpectre11 commented Aug 8, 2019 •

edited

Loading